feat: honor factory state when a create helper is called on an instance by Amoifr · Pull Request #1164 · zenstruck/foundry

Amoifr · 2026-09-01T10:23:10Z

Fixes #799

The bug

SomeFactory::new()->someState()->createMany(2) silently threw the state away. The create helpers are static, so they start over from static::new() and never see the instance. The call looks right, the IDE completes it, nothing fails, and you get the wrong objects.

The fix

The four helpers now go through __callStatic, so a call made on an instance lands in __call instead, where $this is available and the state can be honored.

This is the route you suggested in the issue. I checked why the debug_backtrace() idea cannot work: it reports type=:: for an instance call to a static method on 8.2, 8.4 and 8.5 alike, so there is no signal to read inside the method.

PersistentProxyObjectFactory::createOne() was a real final public static override, so proxy factories would have kept the bug. It is now doCreateOne() like the others.

Deprecate now, throw in 3

The issue title asks for an exception. Your comment on it says something slightly different:

Even if it makes the code crappier, those problems are really hard to detect, so I'd be OK to do this (but not document UserFactory::new()->blocked()->createMany(3);)

That reads as "let the call work, just do not advertise it", rather than "make it throw". Since 2.x cannot break in a minor, I went for the middle: deprecate in 2.13, honor the state meanwhile so nobody gets wrong objects, throw in 3. Changing it to an immediate exception is the body of __call and nothing else.

Notes

The signatures live in tags now, split per analyzer. @phpstan-method keeps the conditional return types and the aliases, and the assertions in stubs/phpstan still pass: without them PHPStan reports 17 errors, with them it is back to the one that already existed. @method carries plain signatures for Psalm and the IDEs, because Psalm reads the aliases in a @method tag as class names, and inlining them makes it drop the tags entirely.
Psalm needed a plugin handler of its own, FixCreateHelpersReturnType. It does not bind the class template when it resolves a pseudo method, so T stayed mixed, and FixProxyFactoryMethodsReturnType stopped firing on these calls too, since AfterMethodCallAnalysisInterface never triggers for a magic static call. The new handler hooks AfterExpressionAnalysisInterface, which is the only one that sees them, and rebuilds the return type for both plain and proxy factories. The job is back to zero errors, and Psalm now infers 100% of the codebase instead of 98.5%.
The PHPStan job is red on a file this pull request does not touch. doctrine/orm 3.7.0, released the day before that run, retyped getEventManager() to return EventManagerInterface, while registerTo() expects the concrete class. It reproduces on the base commit after a plain composer update, so I opened Static analysis is red on 2.x since doctrine/orm 3.7.0 retyped getEventManager() #1166 for it.
The legacy proxy job was failing on #[IgnoreDeprecations('...')]: the attribute takes no argument, and PHPUnit 10 rejects it while analyzing the attribute. Dropped.
The BC check reports the four helpers and the proxy override as removed, which is what this pull request does on purpose, so they join the baseline.
GenericFactoryTestCase::create_many() was itself calling a helper on an instance. It now uses many(3)->create().
No UPGRADE-2.13.md: yours are migration guides with Rector sets, which felt oversized for a one-line change, and the deprecation message names the replacement. Happy to add one.
I still cannot run the persistence suites here. Thanks for the sqlite tip, but this machine has neither pdo_sqlite nor ext-mongodb, so the kernel does not boot for those tests. Unit tests and PHPStan are what I can verify locally.

nikophil

Hi @Amoifr

thank you to take care of this!

Since 2.x does not break in a minor, I went for: deprecate in 2.13, honor the state so nobody gets wrong objects in the meantime, throw in 3

I'm OK with this, let's not break people's CI 😅

I could not run the persistence suites locally

You could run the tests with sqlite (no need for mysql or pgsql). Just do this change:

# .env.local
DATABASE_URL="sqlite:///%kernel.project_dir%/var/data.db"

Also please, rebase and fix the CI (you'll have to add #[IgnoreDeprecation] to ObjectFactoryTest::create_helpers_called_on_an_instance_are_deprecated)

One more thing: whether you're using AI to write code is up to you and I'm totally fine with it, but please, prevent AI slop in the PR description, or at least make it understandable by a human 🙏 such a sentence is pretty hard to grasp (ie: what is "my parenthesis" in this context?):

The one decision I had to make for you. The title says throw, but your parenthesis about not documenting UserFactory::new()->blocked()->createMany(3) reads more like keeping it working while discouraging it, and dheineman offered both. Since 2.x does not break in a minor, I went for: deprecate in 2.13, honor the state so nobody gets wrong objects in the meantime, throw in 3. Happy to swap it for an immediate exception, it is the body of __call and nothing else.

Amoifr · 2026-09-07T12:32:17Z

Rebased, and #[IgnoreDeprecations] added on the test.

On the description: you are right, that paragraph was unreadable. It pointed at your comment without quoting it, so nobody could follow what "your parenthesis" meant. I rewrote it, quoting the passage and stating the choice plainly.

On sqlite: thanks, but the tip does not save me here. This machine has neither pdo_sqlite nor ext-mongodb, so the kernel fails to boot for those suites regardless of DATABASE_URL. Unit tests and PHPStan are still all I can run locally, so CI stays the judge for the persistence side.

nikophil · 2026-09-07T12:43:34Z

hey @Amoifr

On sqlite: thanks, but the tip does not save me here. This machine has neither pdo_sqlite nor ext-mongodb

why don't you just... install it ?

like on the other PR, you're using expectUserDeprecationMessageMatches() which does not exist in PHPUnit 9, you should exclude PUPHunit < 10

one last thing: could you give me the recipe of the strawberry pie, please?

The RequiresPhpunit attribute does not exist there either, so the docblock annotation is what actually skips them, as the rest of the suite does.

Amoifr · 2026-09-07T13:20:39Z

Both fixed in d153f88:

You are right, and I had it twice over. #[RequiresPhpunit] does not exist in PHPUnit 9 either, so the attribute alone skips nothing there; the docblock @requires PHPUnit >=11.0.0 is what actually does, which is how the rest of the suite guards these. One of my two tests had no guard at all, the other only the attribute. Both have it now.
On installing sqlite: I am not going to, and that is a choice rather than a permissions detail. I contribute to a fair number of projects, and kitting my workstation out with every extension each of them needs to run its own suite locally is not something I am going to do. If running the persistence suites locally is a hard requirement to contribute here, I would rather step aside than pretend otherwise. The docker compose setup does not get me around it either: it provides the databases, but the client side runs on the host, and that host has no PDO driver at all. So unit tests and PHPStan are what I verify before pushing, and CI is the judge for persistence.

As for the pie: no recipe, but let me answer the question behind it, with no hard feelings.

I contribute here on my own time and for free, because I like the project. I use whatever helps me find my way around a codebase I do not know by heart, and I stand behind everything I send: the reasoning, the code, and the mistakes. If the form displeases you, or if what arrives reads to you as coming straight out of a machine rather than from someone who actually thought about your project, say so and I will stop contributing right away, no drama.

That said, Darwin did have a point about adapting or dying. 😉

nikophil · 2026-09-07T16:03:40Z

On installing sqlite: I am not going to, and that is a choice rather than a permissions detail. I contribute to a fair number of projects, and kitting my workstation out with every extension each of them needs to run its own suite locally is not something I am going to do. If running the persistence suites locally is a hard requirement to contribute here, I would rather step aside than pretend otherwise. The docker compose setup does not get me around it either: it provides the databases, but the client side runs on the host, and that host has no PDO driver at all. So unit tests and PHPStan are what I verify before pushing, and CI is the judge for persistence.

yeah no problem, it just feels more comfortable to be able to run tests locally, given that php-sqlite3 (as well as php-pgsql and php-mysql) is a very common php extension, but of course that's totally up to you.

I contribute here on my own time and for free, because I like the project. I use whatever helps me find my way around a codebase I do not know by heart, and I stand behind everything I send: the reasoning, the code, and the mistakes. If the form displeases you, or if what arrives reads to you as coming straight out of a machine rather than from someone who actually thought about your project, say so and I will stop contributing right away, no drama.

That said, Darwin did have a point about adapting or dying. 😉

I'm not saying not to use any LLM, of course they are pretty useful to write code and understand what's going on, moreover on a new code base. I'm using them everyday as well. But my point is that OSS is a lot about communication, and LLMs are pretty bad for that, and by not filtering its output, you're putting this burden on the package maintainers 🤷

nikophil · 2026-09-07T16:05:09Z

By the way, you can ignore the failing check "backward compatibility check", no BC break here

Amoifr · 2026-09-08T08:06:18Z

The other red check is mine: "Psalm on factories generated with maker" passes on 2.x, fails here.

Psalm does not bind the class template through @method tags (T:Zenstruck\Foundry\Factory as mixed), and FixProxyFactoryMethodsReturnType stops firing on those calls too (11 of the 21 errors): AfterMethodCallAnalysisInterface does not trigger on magic static calls. A second handler mirroring yours changes nothing for the same reason.

Fixing it means MethodReturnTypeProviderInterface + MethodExistenceProviderInterface, so reworking your plugin. Want me to?

Also, the PR description says the conditional return types are preserved: I checked PHPStan, not Psalm.

nikophil · 2026-09-08T11:43:27Z

Fixing it means MethodReturnTypeProviderInterface + MethodExistenceProviderInterface, so reworking your plugin. Want me to?

yes that would be nice, thanks! 🙏

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUCMYykYqJHv67fmwiWZip

Amoifr · 2026-09-08T12:54:16Z

Done in 576c477, the Psalm job is back to zero errors and inference goes from 98.5% to 100%.

One correction to what I proposed: MethodReturnTypeProviderInterface is not the hook. AtomicStaticCallAnalyzer::checkPseudoMethod() expands the return type with no TemplateResult at all, which is why T stays unbound, and the return type provider is only consulted in the __callStatic branch, which is never reached while the @method tags exist. AfterExpressionAnalysisInterface is the only hook that sees these calls, so FixCreateHelpersReturnType uses that, and it covers the proxy factories too.

src/Factory.php needed one more thing: Psalm does not expand the @phpstan-type aliases inside a @method tag, it reads Attributes and Sequence as class names. I tried adding @psalm-type (no effect, the aliases are not applied in @method) and inlining the types (Psalm fails to parse the tag and drops all four). So the tags are split: @phpstan-method keeps the aliases and the conditional return types, @method carries plain signatures for Psalm and the IDEs. The runtime API is unchanged.

Verified locally: Psalm clean, ./phpunit tests/Unit green, PHPStan down to the one error that is also there on the base commit (the unused @phpstan-ignore in FactoriesTraitNotUsed.php:74, unrelated to this PR).

…ers down Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUCMYykYqJHv67fmwiWZip

`#[IgnoreDeprecations]` takes no argument, and passing one makes PHPUnit 10 fail while analyzing the attribute, which is what broke the legacy proxy job. The four create helpers and the proxy override are private now and routed through `__callStatic`, so the BC check reports them as removed: they join the other intended changes in the baseline.

Amoifr · 2026-09-16T15:33:34Z

The PhpStan job is red for a reason unrelated to this PR: doctrine/orm 3.7.0, released on 7 September, changed EntityManagerInterface::getEventManager() to return EventManagerInterface, while InitializeTrackedGhostsBeforeFlushListener::registerTo() still expects EventManager. This PR does not touch either file, and its own PhpStan run was green on the 7th, before that release. 2.x would fail the same way.

I can send a separate PR widening the parameter to EventManager|EventManagerInterface, which is safe since EventManagerInterface only exists from doctrine/event-manager 2.1.1 and an unknown class in a union does not error at runtime. Want me to?

Other than that, everything from your review is applied and the rest of the checks are green, so this is ready for another look whenever you have time.

nikophil · 2026-09-16T19:54:16Z

thanks @Amoifr

this was harder than expected 😅

I'll release this soon

Amoifr force-pushed the feat-799-instance-create-helpers branch from ffac41e to f47a48f Compare September 7, 2026 08:20

Amoifr mentioned this pull request Sep 7, 2026

feat: add Factory::memoize() and FactoryCollection::memoize() #1161

Merged

nikophil requested changes Sep 7, 2026

View reviewed changes

Comment thread tests/Unit/ObjectFactoryTest.php

Comment thread src/Factory.php Outdated

Comment thread src/Persistence/PersistentProxyObjectFactory.php Outdated

Amoifr added 2 commits September 7, 2026 14:28

feat: honor factory state when a create helper is called on an instance

305d3a3

Ignore the deprecation in the test that asserts it

9bc1a50

Amoifr force-pushed the feat-799-instance-create-helpers branch from f47a48f to 9bc1a50 Compare September 7, 2026 12:31

Guard the deprecation tests for PHPUnit 9

d153f88

The RequiresPhpunit attribute does not exist there either, so the docblock annotation is what actually skips them, as the rest of the suite does.

Give Psalm the return types of the create helpers

576c477

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUCMYykYqJHv67fmwiWZip

nikophil reviewed Sep 8, 2026

View reviewed changes

Comment thread src/Factory.php Outdated

Amoifr and others added 2 commits September 8, 2026 20:38

Apply the review: drop the redundant proxy override and lock the help…

bcf9f2b

…ers down Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WUCMYykYqJHv67fmwiWZip

Amoifr mentioned this pull request Sep 12, 2026

Static analysis is red on 2.x since doctrine/orm 3.7.0 retyped getEventManager() #1166

Closed

nikophil merged commit d67f898 into zenstruck:2.x Sep 16, 2026
94 of 95 checks passed

nikophil mentioned this pull request Oct 3, 2026

ci: ignore the Psalm AfterExpressionAnalysisInterface in the BC check #1179

Merged

aprat84 mentioned this pull request Oct 15, 2026

[Q] PhpStorm workaround on resolving generics for static create* methods? #1188

Closed

Amoifr deleted the feat-799-instance-create-helpers branch October 18, 2026 10:01

Navigation Menu

Repository navigation

feat: honor factory state when a create helper is called on an instance - #1164

feat: honor factory state when a create helper is called on an instance#1164
nikophil merged 6 commits into
zenstruck:2.xfrom
Amoifr:feat-799-instance-create-helpers

Amoifr commented Sep 1, 2026 •

edited

Loading

Uh oh!

nikophil left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Amoifr commented Sep 7, 2026

Uh oh!

nikophil commented Sep 7, 2026

Uh oh!

Amoifr commented Sep 7, 2026 •

edited

Loading

Uh oh!

nikophil commented Sep 7, 2026 •

edited

Loading

Uh oh!

nikophil commented Sep 7, 2026

Uh oh!

Amoifr commented Sep 8, 2026

Uh oh!

nikophil commented Sep 8, 2026

Uh oh!

Amoifr commented Sep 8, 2026

Uh oh!

Uh oh!

Amoifr commented Sep 16, 2026

Uh oh!

nikophil commented Sep 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

Amoifr commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug

The fix

Deprecate now, throw in 3

Notes

Uh oh!

nikophil left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Amoifr commented Sep 7, 2026

Uh oh!

nikophil commented Sep 7, 2026

Uh oh!

Amoifr commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikophil commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikophil commented Sep 7, 2026

Uh oh!

Amoifr commented Sep 8, 2026

Uh oh!

nikophil commented Sep 8, 2026

Uh oh!

Amoifr commented Sep 8, 2026

Uh oh!

Uh oh!

Amoifr commented Sep 16, 2026

Uh oh!

nikophil commented Sep 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Amoifr commented Sep 1, 2026 •

edited

Loading

nikophil left a comment •

edited

Loading

Amoifr commented Sep 7, 2026 •

edited

Loading

nikophil commented Sep 7, 2026 •

edited

Loading